Add DOMXPath::$enableRegisterNodeNS property #779
Conversation
There was a problem hiding this comment.
The property handlers need to be exported. The build fails because of undefined identifiers.
DaveRandom
force-pushed
the
feature/domxpath-registernodens-prop
branch
from
ef097a1 to
f62a89e
Compare
ext/dom/xpath.c
Outdated
There was a problem hiding this comment.
Isn't this SEPARATE_ZVAL(&newval);? Not sure, but there must be an idiom for this :)
There was a problem hiding this comment.
SEPARATE_ZVAL would heap-allocate the copy, requiring it to be manually destroyed after the conversion. I'm not necessarily against this, but it does seem a little inefficient. I'm not aware of a macro that would gain us anything here, although I'm happy to be corrected.
Note also that this method is the same as the method used in other DOM property write handlers - do as the Romans do, and all that :-)
There was a problem hiding this comment.
I'd trust the Romans too :)
There was a problem hiding this comment.
Just use zend_is_true instead. No copies required.
There was a problem hiding this comment.
Oh, good point. Will push updated patch later today.
DaveRandom
force-pushed
the
feature/domxpath-registernodens-prop
branch
from
f62a89e to
a13e68d
Compare
Fix for bug #55700
DaveRandom
force-pushed
the
feature/domxpath-registernodens-prop
branch
from
a13e68d to
06028e4
Compare
|
Patch is now updated according to comments. |
|
I just realised this is extremely old, in light of that, and since this targets a security fix only branch, and since a patch against a supported branch would have to be different, I'm closing this PR. Please take this action as encouragement to open a clean PR against a supported branch. |
6 participants
Fix for bug #55700.
As this is a small feature with no BC implications, this patch targets 5.6.